-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Add member count for groups in VaultDetail view #321
Conversation
WalkthroughThe changes update both the backend and frontend to support enhanced group functionality by introducing member count tracking. In the backend, Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (9)
backend/src/main/java/org/cryptomator/hub/api/GroupDto.java (1)
10-13
: Add validation for memberCount parameter.The constructor should validate that memberCount is not negative to ensure data integrity.
GroupDto(@JsonProperty("id") String id, @JsonProperty("name") String name, @JsonProperty("memberCount") int memberCount) { super(id, Type.GROUP, name, null); + if (memberCount < 0) { + throw new IllegalArgumentException("Member count cannot be negative"); + } this.memberCount = memberCount; }backend/src/main/java/org/cryptomator/hub/api/AuthorityDto.java (1)
38-44
: Consider moving member count logic to service layer.The DTO layer should focus on data transformation. Consider moving the member count retrieval logic to a dedicated service class to maintain better separation of concerns.
backend/src/main/java/org/cryptomator/hub/entities/Group.java (2)
23-25
: Avoid repository injection in entity classes.Injecting repositories into entity classes breaks the JPA entity model and could cause serialization issues. Consider moving this logic to a service layer.
-@Inject -transient Repository groupRepo; -public int getMemberCount() { - return groupRepo.countMembers(this.getId()); -}Also applies to: 34-36
40-45
: Optimize member count query for large groups.Using
SIZE
on a collection could be inefficient for large groups. Consider using a COUNT query instead.public int countMembers(String groupId) { return getEntityManager() - .createQuery("SELECT SIZE(g.members) FROM Group g WHERE g.id = :groupId", Integer.class) + .createQuery("SELECT COUNT(m) FROM Group g JOIN g.members m WHERE g.id = :groupId", Long.class) .setParameter("groupId", groupId) - .getSingleResult(); + .getSingleResult().intValue(); }backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java (1)
5-5
: Remove unused imports.The following imports are not used in the code:
jakarta.ws.rs.core.Response
java.util.HashMap
java.util.Map
Also applies to: 17-18
backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.java (1)
56-64
: Consider adding null check for the authority parameter.While the implementation is good, it could benefit from defensive programming.
private AuthorityDto convertToDto(Authority a) { + if (a == null) { + throw new IllegalArgumentException("authority cannot be null"); + } if (a instanceof User u) { return UserDto.justPublicInfo(u); } else if (a instanceof Group g) { int memberCount = (int) userRepo.getEffectiveGroupUsers(g.getId()).count(); return new GroupDto(g.getId(), g.getName(), memberCount); } else { throw new IllegalStateException("authority is not of type user or group"); } }frontend/src/components/SearchInputGroup.vue (1)
31-33
: Consider adding aria-label for screen readers.The member count display could benefit from improved accessibility.
-<span v-if="'memberCount' in item && item.memberCount !== undefined" class="ml-auto text-xs text-gray-500 italic"> +<span v-if="'memberCount' in item && item.memberCount !== undefined" + class="ml-auto text-xs text-gray-500 italic" + :aria-label="`${item.memberCount} ${t('vaultDetails.sharedWith.members')}`"> {{ item.memberCount }} {{ t('vaultDetails.sharedWith.members') }} </span>frontend/src/common/backend.ts (1)
90-96
: Consider adding retry logic for the member count API call.The getMemberCount implementation could be more resilient to temporary network issues.
class GroupService { public async getMemberCount(groupId: string): Promise<number> { + const maxRetries = 3; + const retryDelay = 1000; // 1 second + + for (let attempt = 1; attempt <= maxRetries; attempt++) { + try { return axiosAuth.get<UserDto[]>(`/groups/${groupId}/effective-members`) .then(response => response.data.length) .catch(() => 0); + } catch (error) { + if (attempt === maxRetries) return 0; + await new Promise(resolve => setTimeout(resolve, retryDelay)); + } + } + return 0; } }frontend/src/components/VaultDetails.vue (1)
498-518
: Consider implementing debouncing for the search functionality.The search functionality could benefit from debouncing to prevent excessive API calls.
+import { debounce } from '../common/util'; + +const debouncedSearch = debounce(async (query: string) => { + const results = await backend.authorities.search(query); + return results; +}, 300); + async function searchAuthority(query: string): Promise<(AuthorityDto & { memberCount?: number })[]> { - const results = await backend.authorities.search(query); + const results = await debouncedSearch(query); const filtered = results.filter(authority => !members.value.has(authority.id)); const enhanced = await Promise.all( filtered.map(async authority => { if (authority.type === "GROUP") { try { const count = await backend.groups.getMemberCount(authority.id); return { ...authority, memberCount: count }; } catch (error) { return { ...authority, memberCount: 0 }; } } return authority; }) ); return enhanced.sort((a, b) => a.name.localeCompare(b.name)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/src/main/java/org/cryptomator/hub/api/AuthorityDto.java
(2 hunks)backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.java
(4 hunks)backend/src/main/java/org/cryptomator/hub/api/GroupDto.java
(1 hunks)backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java
(3 hunks)backend/src/main/java/org/cryptomator/hub/entities/Group.java
(3 hunks)frontend/src/common/backend.ts
(2 hunks)frontend/src/components/SearchInputGroup.vue
(2 hunks)frontend/src/components/VaultDetails.vue
(4 hunks)frontend/src/i18n/en-US.json
(1 hunks)
🔇 Additional comments (12)
frontend/src/i18n/en-US.json (1)
260-260
: New i18n Entry for Member Count
The new entry"vaultDetails.sharedWith.members": "Members"
is correctly added and follows the naming convention established in the file. This key will assist the UI in displaying the group member count as intended. Ensure that corresponding translation updates are made in other language files if applicable.backend/src/main/java/org/cryptomator/hub/api/GroupDto.java (1)
8-8
: LGTM! Clean implementation of member count functionality.The implementation correctly:
- Uses final field for immutability
- Provides proper JSON serialization annotations
- Implements a clean static factory method
Also applies to: 15-22
backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.java (3)
27-28
: LGTM! Clean dependency injection.The injection of
User.Repository
is well-placed and follows dependency injection best practices.
37-40
: LGTM! Clean stream operation chain.The stream operation chain is well-structured and maintains good readability.
51-53
: LGTM! Consistent with the search method implementation.The implementation maintains consistency with the search method's approach.
frontend/src/components/SearchInputGroup.vue (3)
11-17
: LGTM! Clean conditional rendering.The ComboboxInput implementation is well-structured with clear conditions and attributes.
19-24
: LGTM! Accessible and responsive layout.The selected item display is well-implemented with proper truncation and responsive design.
55-55
: LGTM! Proper i18n setup.The i18n integration is well-implemented with proper scope configuration.
Also applies to: 63-63
frontend/src/common/backend.ts (2)
87-87
: LGTM! Type-safe member count.The memberCount field is properly typed as a number.
398-398
: LGTM! Clean service registration.The GroupService is properly registered in the services object.
frontend/src/components/VaultDetails.vue (2)
51-54
: LGTM! Accessible and localized member count display.The member count display is well-implemented with proper localization.
340-348
: LGTM! Efficient computed property implementation.The groupMemberCounts computed property is well-implemented with proper type checking.
@Inject | ||
static User.Repository userRepo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid static injection for thread safety.
Using static injection for repositories can cause issues in multi-threaded environments and makes testing more difficult. Consider passing the repository as a parameter to the fromEntity method.
-@Inject
-static User.Repository userRepo;
static AuthorityDto fromEntity(Authority a) {
+static AuthorityDto fromEntity(Authority a, User.Repository userRepo) {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
@Inject | |
static User.Repository userRepo; | |
// Removed static injection for thread safety and better separation of concerns | |
// (Removed the following lines) | |
// @Inject | |
// static User.Repository userRepo; | |
static AuthorityDto fromEntity(Authority a, User.Repository userRepo) { | |
// method implementation that makes use of the passed userRepo | |
// ... | |
} |
List<Group> groups = groupRepo.findAll().list(); | ||
return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize group listing to avoid N+1 query.
The current implementation makes a separate query for each group's member count. Consider fetching all member counts in a single query.
public List<GroupDto> getAll() {
- List<Group> groups = groupRepo.findAll().list();
- return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList();
+ var groups = groupRepo.findAll().list();
+ var memberCounts = getEntityManager()
+ .createQuery("SELECT g.id, COUNT(m) FROM Group g LEFT JOIN g.members m GROUP BY g.id", Object[].class)
+ .getResultList()
+ .stream()
+ .collect(Collectors.toMap(row -> (String) row[0], row -> ((Long) row[1]).intValue()));
+ return groups.stream()
+ .map(group -> GroupDto.fromEntity(group, memberCounts.getOrDefault(group.getId(), 0)))
+ .toList();
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
List<Group> groups = groupRepo.findAll().list(); | |
return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList(); | |
public List<GroupDto> getAll() { | |
- List<Group> groups = groupRepo.findAll().list(); | |
- return groups.stream().map(group -> GroupDto.fromEntity(group, groupRepo.countMembers(group.getId()))).toList(); | |
+ var groups = groupRepo.findAll().list(); | |
+ var memberCounts = getEntityManager() | |
+ .createQuery("SELECT g.id, COUNT(m) FROM Group g LEFT JOIN g.members m GROUP BY g.id", Object[].class) | |
+ .getResultList() | |
+ .stream() | |
+ .collect(Collectors.toMap(row -> (String) row[0], row -> ((Long) row[1]).intValue())); | |
+ return groups.stream() | |
+ .map(group -> GroupDto.fromEntity(group, memberCounts.getOrDefault(group.getId(), 0))) | |
+ .toList(); | |
} |
const fetchedMembers = await backend.vaults.getMembers(props.vaultId); | ||
for (const member of fetchedMembers) { | ||
if (member.type === "GROUP") { | ||
try { | ||
const count = await backend.groups.getMemberCount(member.id); | ||
members.value.set(member.id, { ...member, memberCount: count }); | ||
} catch (error) { | ||
members.value.set(member.id, { ...member, memberCount: 0 }); | ||
} | ||
} else { | ||
members.value.set(member.id, member); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using Promise.allSettled for parallel member count fetching.
The current implementation fetches member counts sequentially, which could be optimized.
- const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
- for (const member of fetchedMembers) {
- if (member.type === "GROUP") {
- try {
- const count = await backend.groups.getMemberCount(member.id);
- members.value.set(member.id, { ...member, memberCount: count });
- } catch (error) {
- members.value.set(member.id, { ...member, memberCount: 0 });
- }
- } else {
- members.value.set(member.id, member);
- }
- }
+ const fetchedMembers = await backend.vaults.getMembers(props.vaultId);
+ const memberPromises = fetchedMembers.map(async member => {
+ if (member.type === "GROUP") {
+ const count = await backend.groups.getMemberCount(member.id)
+ .catch(() => 0);
+ return { ...member, memberCount: count };
+ }
+ return member;
+ });
+ const results = await Promise.allSettled(memberPromises);
+ results.forEach((result, index) => {
+ if (result.status === 'fulfilled') {
+ members.value.set(result.value.id, result.value);
+ }
+ });
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const fetchedMembers = await backend.vaults.getMembers(props.vaultId); | |
for (const member of fetchedMembers) { | |
if (member.type === "GROUP") { | |
try { | |
const count = await backend.groups.getMemberCount(member.id); | |
members.value.set(member.id, { ...member, memberCount: count }); | |
} catch (error) { | |
members.value.set(member.id, { ...member, memberCount: 0 }); | |
} | |
} else { | |
members.value.set(member.id, member); | |
} | |
} | |
const fetchedMembers = await backend.vaults.getMembers(props.vaultId); | |
const memberPromises = fetchedMembers.map(async member => { | |
if (member.type === "GROUP") { | |
const count = await backend.groups.getMemberCount(member.id) | |
.catch(() => 0); | |
return { ...member, memberCount: count }; | |
} | |
return member; | |
}); | |
const results = await Promise.allSettled(memberPromises); | |
results.forEach((result, index) => { | |
if (result.status === 'fulfilled') { | |
members.value.set(result.value.id, result.value); | |
} | |
}); |
This PR addresses #173 by displaying the number of members for shared groups in the VaultDetail view.
The member count appears as italicized gray text (e.g., 3 Members) next to group names.
This information is visible both in the search results and the "Shared with" list, helping distinguish groups from individual users.


